[ZEPPELIN-2698] Apply KerberosInterpreter to JDBCInterpreter #2443

prabhjyotsingh · 2017-06-27T09:05:24Z

What is this PR for?

This is to apply new KerberosInterpreter mechanism to JDBCInterpreter for relogin from keytab, instead of on fail relogin.

What type of PR is it?

[Refactoring]

What is the Jira issue?

ZEPPELIN-2698

How should this be tested?

In JDBC interpreter setting add following properties

zeppelin.jdbc.auth.type = KERBEROS
zeppelin.jdbc.principal = principal value
zeppelin.jdbc.keytab.location = keytab location

Now try and run any of hive's query (say show tables) it should return with valid results.
Again, wait for this kerberos ticket to expire (usually its 24hrs) then try the above again, and it should work.

Questions:

Does the licenses files need update? N/A
Is there breaking changes for older versions? N/A
Does this needs documentation? N/A

prabhjyotsingh · 2017-06-30T14:26:24Z

@felixcheung @zjffdu can you help review this as well?

zjffdu · 2017-06-30T14:41:50Z

This PR LGTM. Just one comment on the base class KerberosInterpreter. I think startKerberosLoginThread and shutdownExecutorService should be called in KerberosInterpreter's open and close method rather than ask the sub class to call them. The sub class only implement 2 abstract methods:

isKerboseEnabled
runKerberosLogin

Which means

public void open() {
   if (isKerboseEnabled()) {
      startKerberosLoginThread()
   }
}

public void close() {
   if (isKerboseEnabled()) {
      shutdownExecutorService()
   }
}

…nt class

prabhjyotsingh · 2017-06-30T15:32:58Z

@zjffdu sure, that does make sense.

prabhjyotsingh · 2017-07-04T05:47:47Z

@zjffdu I've made the suggested changes. Let me know if I'm missing something else as well.

zjffdu · 2017-07-05T05:27:56Z

zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/KerberosInterpreter.java

+  }
+
+  private String getKerberosRefreshInterval() {
    if (System.getenv("KERBEROS_REFRESH_INTERVAL") == null) {


I think we need to add javadoc for KerberosInterpreter.java to explain the internal mechanism of it. So basically I believe user need to do 3 things to extend KerberosInterpreter.

implement runKerberosLogin

implement isKerboseEnabled

define KERBEROS_REFRESH_INTERVAL in interpreter setting. Maybe we could also add abstract method like getKerborseRefreshInterval to enforce its sub class to implement it.

Sure, have added isKerboseEnabled in javadoc.
For KERBEROS_REFRESH_INTERVAL I think all interpreters can share a common its value defined in zeppelin-env.sh (https://github.com/apache/zeppelin/blob/master/conf/zeppelin-env.sh.template#L58-L59) instead of having it at interpreter level, and increasing the complexity (number of lines) of interpreter config.

zjffdu · 2017-07-05T05:29:01Z

jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java

+  protected boolean runKerberosLogin() {
+    try {
+      UserGroupInformation.AuthenticationMethod authType = JDBCSecurityImpl.getAuthtype(property);
+      if (authType.equals(KERBEROS)) {


authType is not needed to check as runKerberosLogin would be only called when kerberos is enabled.

Yes, make sense, removed unnecessary if condition.

zjffdu · 2017-07-05T05:36:51Z

shell/src/main/java/org/apache/zeppelin/shell/ShellInterpreter.java

    super(property);
  }

  @Override


I think we can move createSecureConfiguration from ShellSecurityImpl to ShellInterpreter, then ShellSecurityImpl could be deleted

Sure, done.

LAUNCH_KERBEROS_REFRESH_INTERVAL to KERBEROS_REFRESH_INTERVAL

…alues

zjffdu · 2017-07-06T06:50:40Z

LGTM

prabhjyotsingh · 2017-07-06T13:55:43Z

@zjffdu Thank you for the review. CI is green, will merge if no more discussion.

@zjffdu

### What is this PR for? This is to apply new KerberosInterpreter mechanism to JDBCInterpreter for relogin from keytab, instead of on fail relogin. ### What type of PR is it? [Refactoring] ### What is the Jira issue? * [ZEPPELIN-2698](https://issues.apache.org/jira/browse/ZEPPELIN-2698) ### How should this be tested? In JDBC interpreter setting add following properties - zeppelin.jdbc.auth.type = KERBEROS - zeppelin.jdbc.principal = principal value - zeppelin.jdbc.keytab.location = keytab location Now try and run any of hive's query (say `show tables`) it should return with valid results. Again, wait for this kerberos ticket to expire (usually its 24hrs) then try the above again, and it should work. ### Questions: * Does the licenses files need update? N/A * Is there breaking changes for older versions? N/A * Does this needs documentation? N/A Author: Prabhjyot Singh <[email protected]> Author: prabhjyotsingh <[email protected]> Closes apache#2443 from prabhjyotsingh/ZEPPELIN-2698 and squashes the following commits: 835b4bd [Prabhjyot Singh] check for invalid user input; in case of error fall back to default values a5a54d4 [Prabhjyot Singh] runKerberosLogin block should return false 5823727 [Prabhjyot Singh] change schedule to submit so it runs without wait for the first time. LAUNCH_KERBEROS_REFRESH_INTERVAL to KERBEROS_REFRESH_INTERVAL 7fe883c [Prabhjyot Singh] @zjffdu review comments 7f8b867 [prabhjyotsingh] call `startKerberosLoginThread` and `shutdownExecutorService` in parent class 57ea80c [Prabhjyot Singh] apply KerberosInterpreter to JDBCInterpreter

@zjffdu

### What is this PR for? This is to apply new KerberosInterpreter mechanism to JDBCInterpreter for relogin from keytab, instead of on fail relogin. ### What type of PR is it? [Refactoring] ### What is the Jira issue? * [ZEPPELIN-2698](https://issues.apache.org/jira/browse/ZEPPELIN-2698) ### How should this be tested? In JDBC interpreter setting add following properties - zeppelin.jdbc.auth.type = KERBEROS - zeppelin.jdbc.principal = principal value - zeppelin.jdbc.keytab.location = keytab location Now try and run any of hive's query (say `show tables`) it should return with valid results. Again, wait for this kerberos ticket to expire (usually its 24hrs) then try the above again, and it should work. ### Questions: * Does the licenses files need update? N/A * Is there breaking changes for older versions? N/A * Does this needs documentation? N/A Author: Prabhjyot Singh <[email protected]> Author: prabhjyotsingh <[email protected]> Closes apache#2443 from prabhjyotsingh/ZEPPELIN-2698 and squashes the following commits: 835b4bd [Prabhjyot Singh] check for invalid user input; in case of error fall back to default values a5a54d4 [Prabhjyot Singh] runKerberosLogin block should return false 5823727 [Prabhjyot Singh] change schedule to submit so it runs without wait for the first time. LAUNCH_KERBEROS_REFRESH_INTERVAL to KERBEROS_REFRESH_INTERVAL 7fe883c [Prabhjyot Singh] @zjffdu review comments 7f8b867 [prabhjyotsingh] call `startKerberosLoginThread` and `shutdownExecutorService` in parent class 57ea80c [Prabhjyot Singh] apply KerberosInterpreter to JDBCInterpreter

apply KerberosInterpreter to JDBCInterpreter

57ea80c

call startKerberosLoginThread and shutdownExecutorService in pare…

7f8b867

…nt class

zjffdu reviewed Jul 5, 2017

View reviewed changes

prabhjyotsingh added 4 commits July 5, 2017 13:54

@zjffdu review comments

7fe883c

change schedule to submit so it runs without wait for the first time.

5823727

LAUNCH_KERBEROS_REFRESH_INTERVAL to KERBEROS_REFRESH_INTERVAL

runKerberosLogin block should return false

a5a54d4

check for invalid user input; in case of error fall back to default v…

835b4bd

…alues

prabhjyotsingh closed this Jul 6, 2017

prabhjyotsingh reopened this Jul 6, 2017

asfgit closed this in e1f0a32 Jul 7, 2017

prabhjyotsingh deleted the ZEPPELIN-2698 branch May 2, 2022 16:26

[ZEPPELIN-2698] Apply KerberosInterpreter to JDBCInterpreter #2443

[ZEPPELIN-2698] Apply KerberosInterpreter to JDBCInterpreter #2443

Uh oh!

Conversation

prabhjyotsingh commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

What is the Jira issue?

How should this be tested?

Questions:

Uh oh!

prabhjyotsingh commented Jun 30, 2017

Uh oh!

zjffdu commented Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prabhjyotsingh commented Jun 30, 2017

Uh oh!

prabhjyotsingh commented Jul 4, 2017

Uh oh!

zjffdu Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prabhjyotsingh Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

zjffdu Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prabhjyotsingh Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

zjffdu Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

prabhjyotsingh Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

zjffdu commented Jul 6, 2017

Uh oh!

prabhjyotsingh commented Jul 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

prabhjyotsingh commented Jun 27, 2017 •

edited

Loading

zjffdu commented Jun 30, 2017 •

edited

Loading

zjffdu Jul 5, 2017 •

edited

Loading

zjffdu Jul 5, 2017 •

edited

Loading